Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Fix bug where special characters can be used when generating throttle key #216

Merged

Conversation

liamh101
Copy link
Contributor

Bugfix for Issue I reported earlier in the week.

It was possible to bypass login throttle by replacing alphanumeric characters with their special character equivalent.

This fix replaces them after the username has been translated into lowercase.

This will not affect usernames that don't email due to the fact that on a database level, special characters and their alphanumeric equivalent are seen as the same thing.

@taylorotwell
Copy link
Member

So, there is no way to handle this at the MySQL configuration level to disable this behavior of turning special characters into "normal" characters?

@liamh101
Copy link
Contributor Author

MYSQL will translate special characters back to normal characters automatically. So if I try to login as ⓣⓔⓢⓣ@ⓛⓐⓡⓐⓥⓔⓛ.ⓒⓞⓜ, once I reach the database, the special characters are read as normal characters, no matter if the field data is using special characters or not. Both versions equal the same.

However, when you're checking the number of requests in the throttle. PHP sees the special character key I've generated differently from the alphanumeric key. Resetting the throttle requests. That means an attacker can automate a brute force attempted by simply replacing a single letter with its special character counterpart after a certain number of attempts, bypassing the minute lockdown.

@taylorotwell taylorotwell merged commit 7a4397c into laravel:3.x Jan 20, 2022
@ankurk91
Copy link

@taylorotwell

This character conversion can be done via https://github.com/voku/portable-ascii php package (already present in framework core)

use voku\helper\ASCII;

ASCII::to_transliterate(""); // a
ASCII::to_transliterate(""); // 20

I would suggest to introduce a helper in core like Str::transliterate() and use that helper in UI package.
This will keep UI package light and maintainable :)

@liamh101 Thought?

@liamh101
Copy link
Contributor Author

@ankurk91 @taylorotwell I'm happy to investigate/add this in. I'm not particularly proud of my current implementation as it's a bit messy, so this would be a good refinement.

I suggest we keep this in for now though for security reasons.

@liamh101
Copy link
Contributor Author

PR to core change: laravel/framework#40681

@sebastiaanluca
Copy link

I assume this attack vector also applies to Fortify? Specifically the \Laravel\Fortify\LoginRateLimiter::throttleKey() method:

protected function throttleKey(Request $request)
{
    return Str::lower($request->input(Fortify::username())).'|'.$request->ip();
}

@ankurk91
Copy link

ankurk91 commented Feb 2, 2022

The Str::transliterate method has been added to core recently and can be used to fix this issue

@sebastiaanluca
Copy link

The throttleKey() method is a package class method, so not something we can fix in our app directly (unless you override all the Fortify calls). I'm also not sure if we can already use that new transliterate method if Fortify supports all of Laravel 8.x and not just the latest release.

@ankurk91
Copy link

ankurk91 commented Feb 5, 2022

Laravel UI package need to bump framework version in composer.json and can start using the transliterate method

@liamh101
Copy link
Contributor Author

liamh101 commented Feb 5, 2022

Done that here: #219

jessarcher pushed a commit that referenced this pull request Jul 18, 2022
…rottle key (#216)

* Fix bug where special characters can be used when generating throttle keys

* Update ThrottlesLogins.php

* Update ThrottlesLogins.php

Co-authored-by: Liam Hackett <liamh@DESKTOP-RS5AQ35.localdomain>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants